Skip to content

Plugin/TagFix_Maxspeed_AT #2506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from

Conversation

wolfbert
Copy link
Contributor

@wolfbert wolfbert commented May 8, 2025

The plugin contains a number of tests focusing on maxspeed, maxspeed:type and source:maxspeed specific to Austria. It is by no means complete, but will do until further community decisions have been taken.

I've tested the logic locally, but have no Osmose dev environment. I need ids for 6 issue classes (currently numbered 1 to 6) and integration support. Test cases are provided, but have never run (I used a local test harness to feed data). I'd like to provide a German translation following successful integration.

#2503

@Famlam
Copy link
Collaborator

Famlam commented Jun 1, 2025

Your comments suggest code changes have been made but I don't see them here yet, possibly you forgot to push them live?

@wolfbert
Copy link
Contributor Author

wolfbert commented Jun 2, 2025

I comment as I fix the problems, but push after final testing. Should be ready later today. Thanks for the detailed review.

wolfbert added a commit to wolfbert/osmose-backend that referenced this pull request Jun 2, 2025
Removed failing test case (supposed to test for out-of-country data)
wolfbert added a commit to wolfbert/osmose-backend that referenced this pull request Jun 2, 2025
wolfbert added a commit to wolfbert/osmose-backend that referenced this pull request Jun 2, 2025
wolfbert added a commit to wolfbert/osmose-backend that referenced this pull request Jun 2, 2025
wolfbert added a commit to wolfbert/osmose-backend that referenced this pull request Jun 2, 2025
Copy link
Collaborator

@Famlam Famlam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to run the plugin on a part of Austria but it seems the extracts server is down at the moment

@wolfbert wolfbert force-pushed the plugin/TagFix_Maxspeed_AT branch from f603bc7 to 72795e3 Compare June 4, 2025 10:49
@wolfbert
Copy link
Contributor Author

wolfbert commented Jun 4, 2025

Not sure what's wrong, but for some reason the revert of this isn't included yet, it still shows e.g. AT-NOE instead of AT-3

After a lengthy session with ChatGPT, it turned out there were two problems on my side. First, I had push right for upstream, which I have now blocked locally (not that I intentionally used it). Second, though git reported my fork to be in sync, it wasn't and still had the old version of osmose_config.py. I have fixed that and can see the latest version everywhere. Hope that helps.

@wolfbert
Copy link
Contributor Author

wolfbert commented Jun 4, 2025

I tried to run the plugin on a part of Austria but it seems the extracts server is down at the moment

I have tested with three provinces (including Niederösterreich, which has > 500.000 segments). Most errors come from either maxspeed:type without maxspeed or maxspeed:source=zone.

@Famlam
Copy link
Collaborator

Famlam commented Jun 4, 2025

Some false positives:

  1. warning 'sign without maxspeed`' for
<tag k="source:maxspeed" v="sign" />
<tag k="maxspeed:forward" v="70" />
<tag k="maxspeed:backward" v="100" />
  1. Similar story, variable maxspeeds can be fully covered in maxspeed:(*:)conditionals
source:maxspeed=sign
maxspeed:variable=yes
maxspeed:conditional/maxspeed:forward:conditional/... = 80 @ (X); 120 @ (Y)

Probably it's best to disable 303220 if there's other maxspeed:* tags present besides maxspeed:type. (Obviously, make sure not to trigger class 303222 when skipping 303220)

(Also, it may be better to replace [value] without maxspeed by source:maxspeed/maxspeed:type=[value] without maxspeed, as this improves the readability for cases like sign - first example above - or in case there's an unknown source:maxspeed, such as survey, which would now show survey without maxspeed, which is more confusing than source:maxspeed=survey without maxspeed)

@wolfbert
Copy link
Contributor Author

wolfbert commented Jun 4, 2025

1 warning 'sign without maxspeed`' for

This isn't a false positive. It should read source:maxspeed:forward and source:maxspeed:backward, which are obviously different.

Same for number 2. But these are extreme cases not worth dealing with at the moment.

As mentioned, I've run this with 500K segments - and checked the error log.

I can change the message if that's what it takes.

Edit: for 2., it's maxspeed=* and source:maxspeed=* + whatever variable or conditional values.

@Famlam
Copy link
Collaborator

Famlam commented Jun 5, 2025

Ok, I'd personally prefer to avoid requiring unnecessary tagging (if a single tag can indicate one looked to the front and to the back, why tag it twice... but hopefully those people will then use :both_ways instead. Fortunately maxspeed:both_ways isn't used too much, because it'd cause a lot of false positives)

I'll leave it to the others to decide then :)

@wolfbert
Copy link
Contributor Author

wolfbert commented Jun 5, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants